Loosen lifetime bounds on accessing named captures via Index.
#143
Conversation
| @@ -970,11 +970,11 @@ impl<'t> Index<usize> for Captures<'t> { | |||
| /// | |||
| /// # Panics | |||
| /// If there is no group named by the given value. | |||
| impl<'t> Index<&'t str> for Captures<'t> { | |||
| impl<'t, 'i> Index<&'i str> for Captures<'t> { | |||
There was a problem hiding this comment.
Could you please add a bit of documentation here that briefly describes what the 'i lifetime parameter stands for? Thanks! (The 't lifetime should also be documented, e.g., http://doc.rust-lang.org/regex/regex/struct.FindMatches.html)
There was a problem hiding this comment.
Good point; thanks.
While I'm editing the docs: I noticed that the str can't outlive the Captures if it's obtained through either Index impl; should that be a separate PR? It's a little surprising, so a warning and advice to use methods instead might be nice. (This is part of the definition of the Index trait — normally a[i] is part of a and can't outlive it — so it can't be fixed without changing the Output type to &'t str and breaking compatibility.)
There was a problem hiding this comment.
@jld So sorry I missed this comment! You raise a good point. Including those docs in this PR seems fine to me!
|
Can't believe I missed this! I have one doc nit, but otherwise looks good to me. Thank you! |
The current definition equates the lifetime of the strings being matched/captured and the string used as the capture name. The latter is usually `'static` (string literals), which leads to confusing [borrow-check errors][err] on reasonable-looking code. This change declares the two lifetimes as separate; this should be a strict increase in the set of `impl` instances, so it's a minor version bump. [err][]: https://gist.github.com/jld/476a13a00ad05bd99a63
jld
force-pushed
the
capture-index-lifetime
branch
from
890e0c6
to
18ac6c1
Compare
|
Really sorry for dropping the ball on this. (Also sorry for the rebase / force-push, but I had to fix the version bump.) This version should take care of the doc comment issues. |
| @@ -1,6 +1,6 @@ | |||
| [package] | |||
| name = "regex" | |||
| version = "0.1.48" #:version | |||
| version = "0.1.49" #:version | |||
There was a problem hiding this comment.
Could you remove the version bump please? I typically do them in separate commits with tags using a script.
|
@jld OK, now I'm the one who has dropped the ball! I left a few more nits. If you like, I can pull your branch and make the changes myself---I feel bad about the back and forth! |
|
OK, I just went ahead and merged it in d8fb7f1. Thanks! |
|
Released on crates.io in |
The current definition equates the lifetime of the strings being
matched/captured and the string used as the capture name. The latter
is usually
'static(string literals), which leads to confusing[borrow-check errors][err] on reasonable-looking code.
This change declares the two lifetimes as separate; this should be a
strict increase in the set of
implinstances, so it's a minor versionbump.
[err][]: https://gist.github.com/jld/476a13a00ad05bd99a63